-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add diffuse self-shading functions #1017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Test failures appear to be unrelated, a change in pandas: #1018 |
pvlib/shading.py
Outdated
| return np.degrees(psi_avg) | ||
|
|
||
|
|
||
| def passias_sky_diffuse(masking_angle): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if this signature creates space for other approaches to calculate self-shading. What if the arguments are tilt and 'gcr' and passias_masking_angle was private and called within this function? I doubt that the average "masking angle" is general among other models, I could be wrong.
A thought: how is this visible sky dome calculation done in bifacial_vf? Maybe there's some commonality that could inform the interface here, although we wouldn't re-use code from bifacial_vf. If I recall right, that calculation is done with view factors over small increments of angle.
I have a mild preference for sky_diffuse_passias as the function name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I separated the two functions in anticipation of people wanting to use the "worst-case" angle like SAM does instead of Passias et al.'s average angle. That said, a function that just does 1 - cos(angle/2)^2 does seem strange.
Good idea about bifacial_vf; I haven't had an excuse to get familiar with it before now but am happy to have one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the SAM documentation. I think that the low level functions that calculates the average masking_angle is useful, as would be a function that returns masking_angle at a point along the row's surface. Maybe someone would want to use the masking_angle in combination with other sky diffuse models.
Still chewing on the name passias_sky_diffuse for the effects function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a masking_angle function that calculates the angle for a given point and renamed passias_sky_diffuse to sky_diffuse_passias, but am happy to rename if you have a better alternative.
Maybe someone would want to use the masking_angle in combination with other sky diffuse models.
It occurs to me that the masking angle is also useful for comparing with solar position to determine if direct shading is relevant.
|
|
||
| Notes | ||
| ----- | ||
| The pvlib-python authors believe that Eqn. 9 in [1]_ is incorrect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The equation below isn't equivalent to the published equation? Or, you couldn't reproduce the published equation [9]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter. The published equation [9] also gives different results from evaluating the integral numerically: https://gist.github.com/kanderso-nrel/2c6c3a1853338cdef5b4bbc67092ccc8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm persuaded. Your solution looks correct to me. I computed a few points on the k=1 and k=2 curves to check.
| Panel tilt from horizontal [degrees]. | ||
|
|
||
| gcr : float | ||
| The ground coverage ratio of the array [unitless]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a notion that gcr is sometimes defined as gcr := width * cos(tilt) / pitch for fixed-tilt arrays. Is that a common usage? If so, this parameter description and the one in masking_angle_passias should clarify that width / pitch is meant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience the usual definition of ground coverage ratio is the ratio of module area to ground area, where the "ground area" is left somewhat ambiguous. That's the definition used in Pvsyst, Helioscope and pvlib.tracking.SingleAxisTracker. I think we should stick with gcr = width / pitch. I'd be interested to see where the ratio of projected horizontal width to pitch is used.
pvlib/shading.py
Outdated
| the top of the shading row. SAM assumes the "worst-case" loss where the | ||
| masking angle is calculated for the bottom of the array [1]_. In [2]_ | ||
| the masking angle is calculated as the average across the module height. | ||
|
|
||
| This function, as in [2]_, makes the assumption that sky diffuse | ||
| irradiance is isotropic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Entirely editorial - I'd swap the order of the last and next-to-last sentences, so that the focus stays on the Passias approach.
|
@wholmgren @mikofski any objections to merge? |
wholmgren
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
LGTM too. I made one comment on a trig identity that might be useful, but probably not essential. This is a great addition. Thanks Kevin! |
mikofski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider substituting the trig identity, but not a blocker.
Closes #xxxxdocs/sphinx/source/api.rstfor API changes.docs/sphinx/source/whatsnewfor all changes. Includes link to the GitHub Issue with:issue:`num`or this Pull Request with:pull:`num`. Includes contributor name and/or GitHub username (link with:ghuser:`user`).This implements a modified version of the diffuse self-shading model in Passias and Kallbach (1984) "SHADING EFFECTS IN ROWS OF SOLAR CELL PANELS", which I believe has an error in the equation for the average masking angle (Eq 9). Here is a gist comparing the paper's equation with the one I derived, along with a numerical integration for comparison: https://gist.github.com/kanderso-nrel/2c6c3a1853338cdef5b4bbc67092ccc8. Note that I implemented two versions of the paper's equation to account for an ambiguous minus sign.
If anyone has a Mathematica license and can produce a cleaner version of that equation, I'd be happy to replace it -- that quick and dirty Maxima script in the docstring is about as much as I can do with symbolic solvers.